feat: limit model with WEEKLY, CUSTOM types and time window support#69
Conversation
Add domain constructors and validation for new limit types: - NewLimitWithTimeWindow, NewLimitWithCustomPeriod, NewLimitWithCustomPeriodAndTimeWindow constructors - Time window validation (mismatch, zero-width, overnight support) - Custom period validation (dates required, order, max 5 years) - WEEKLY resetAt calculation (next Monday 00:00 UTC) - DB model conversion for new columns (active_time_start/end, custom_start_date/end_date) - Create/update command handlers for new limit types - MustNewTimeOfDay test helper in internal/testhelper Note: CalculatePeriodKey for WEEKLY/CUSTOM will be added in PR 3 (validation pipeline) along with its tests.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR adds daily active time-window and custom date-range support to limits. The Limit model gains WEEKLY and CUSTOM types plus fields ActiveTimeStart, ActiveTimeEnd, CustomStartDate, CustomEndDate, validation, reset calculations, constructors, and Update handling. PostgreSQL model and repository persist four nullable columns (active_time_start, active_time_end, custom_start_date, custom_end_date) and propagate them through ToEntity/FromEntity. Create and Update commands accept, validate, and parse these inputs. Tests and test helpers are added/updated across model, service, and repository layers. Sequence Diagram(s)sequenceDiagram
participant Client
participant Service as Create/UpdateCmd
participant Validator as ModelValidation
participant Constructor as ModelConstructor
participant Repo as LimitRepository
participant DB as Database
Client->>Service: Execute(input with time window / custom dates)
Service->>Validator: validate presence/format of time window & custom dates
alt validation fails
Validator-->>Service: specific error (time window mismatch / custom dates required / parse error)
Service-->>Client: Error
else validation passes
Service->>Constructor: choose constructor based on inputs
Constructor-->>Service: built Limit entity
Service->>Repo: Create/Update(Limit)
Repo->>DB: INSERT/UPDATE including active_time_* & custom_* columns
DB-->>Repo: OK
Repo-->>Service: persisted Limit
Service-->>Client: Success (created/updated Limit)
end
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Comment |
gandalf-at-lerian
left a comment
There was a problem hiding this comment.
Solid PR. The model decomposition is clean — newLimitBase + specialized constructors avoids repetition without over-abstracting. A few observations:
Half-open interval consistency — both IsWithinTimeWindow and IsWithinCustomPeriod use [start, end) semantics, well-documented and well-tested. The overnight window logic (startMins >= endMins path) is correct. Good.
Update partial semantics — if someone sends only customEndDate (without customStartDate), ValidateCustomPeriod catches it as ErrLimitCustomDatesRequired. Same for time windows. This means partial updates require both fields in the pair, which is the right call for data integrity. Worth a note in the API docs (PR 6) that these are atomic pairs.
Minor asymmetry — CreateLimitInput takes *model.TimeOfDay for time window fields (pre-parsed) but *string for custom dates (parsed in command layer). Not a problem — TimeOfDay is a value object, custom dates are standard RFC3339 — but worth noting for consistency if the team prefers one pattern.
Validate() skips expiry check (comment says "done in constructors") — correct, avoids breaking reads of historical data from DB. Clean separation of creation-time vs read-time validation.
Test coverage is thorough — boundary conditions, overnight windows, partial inputs, defensive nil checks. LGTM 👍
|
This PR is very large (11 files, 2243 lines changed). Consider breaking it into smaller PRs for easier review. |
🔒 Security Scan Results —
|
📊 Unit Test Coverage Report:
|
| Metric | Value |
|---|---|
| Overall Coverage | 83.2% |
| Threshold | 85% |
Coverage by Package
| Package | Coverage |
|---|---|
tracer/internal/adapters/cel |
81.9% |
tracer/internal/adapters/http/in/middleware |
62.0% |
tracer/internal/adapters/http/in |
81.8% |
tracer/internal/adapters/postgres/db |
0.0% |
tracer/internal/adapters/postgres |
74.9% |
tracer/internal/services/cache |
95.6% |
tracer/internal/services/command |
81.5% |
tracer/internal/services/query |
78.3% |
tracer/internal/services/workers |
79.7% |
tracer/internal/services |
40.2% |
tracer/internal/testhelper |
0.0% |
tracer/pkg/clock |
50.0% |
tracer/pkg/contextutil |
100.0% |
tracer/pkg/logging |
100.0% |
tracer/pkg/migration |
89.0% |
tracer/pkg/model |
96.1% |
tracer/pkg/net/http |
88.3% |
tracer/pkg/resilience |
97.8% |
tracer/pkg/sanitize |
87.1% |
tracer/pkg/validation |
50.0% |
tracer/pkg |
96.6% |
Generated by Go PR Analysis workflow
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/adapters/postgres/limit_repository.go (1)
321-331:⚠️ Potential issue | 🟠 MajorPersist
reset_atwhen these fields change.
limit.Update(...)can recalculateResetAtfor custom-period edits, but thisUPDATEnever writes that field back. After a successful update, the in-memory entity and stored row can diverge, leaving the database with a stale reset boundary.💡 Suggested fix
query := sq.Update(r.tableName). Set("name", dbModel.Name). Set("description", dbModel.Description). Set("max_amount", dbModel.MaxAmount). Set("scopes", dbModel.Scopes). Set("status", dbModel.Status). + Set("reset_at", dbModel.ResetAt). Set("active_time_start", dbModel.ActiveTimeStart). Set("active_time_end", dbModel.ActiveTimeEnd). Set("custom_start_date", dbModel.CustomStartDate). Set("custom_end_date", dbModel.CustomEndDate). Set("updated_at", dbModel.UpdatedAt).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/adapters/postgres/limit_repository.go` around lines 321 - 331, The UPDATE builder for r.tableName is missing the reset_at column, so when limit.Update(...) recalculates dbModel.ResetAt for custom-period edits the database isn't updated; add Set("reset_at", dbModel.ResetAt) to the sq.Update(...) chain (where query is built using r.tableName and dbModel) so the recalculated ResetAt is persisted alongside name, description, max_amount, scopes, status, active_time_start/end, custom_start/end and updated_at.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/adapters/postgres/limit_postgresql_model.go`:
- Around line 98-146: After constructing ActiveTimeStart/ActiveTimeEnd and
CustomStartDate/CustomEndDate but before returning the &model.Limit, validate
the paired-field invariants: ensure ActiveTimeStart and ActiveTimeEnd are either
both non-nil or both nil, and likewise CustomStartDate and CustomEndDate; if one
side is set without its pair return a descriptive error. Also validate ordering
(ActiveTimeStart <= ActiveTimeEnd and CustomStartDate <= CustomEndDate) and any
domain span limits your domain requires (e.g., max date span or max time
window); if any check fails return an error instead of the model. Use the
existing variables ActiveTimeStart, ActiveTimeEnd, customStartDate,
customEndDate and the surrounding mapper function that builds and returns
*model.Limit to locate where to insert these checks.
In `@internal/services/command/create_limit_test.go`:
- Around line 495-620: Add a happy-path test that exercises non-nil
ActiveTimeStart/ActiveTimeEnd and CustomStartDate/CustomEndDate to verify
command-layer parsing and repository handoff: in a new subtest (alongside
TestCreateLimitCommand_Execute_PartialCustomPeriod and
TestCreateLimitCommand_Execute_PartialTimeWindow) create a CreateLimitInput with
both ActiveTimeStart and ActiveTimeEnd set and both CustomStartDate and
CustomEndDate set, construct NewCreateLimitCommand with NewMockLimitRepository
and NewMockAuditWriter, set expectations that the repository's create/save
method (the method on NewMockLimitRepository used by Execute) is called once
with a limit containing the supplied times/dates and that
auditWriter.RecordLimitEvent is called once, call
cmd.Execute(context.Background(), input) and assert no error and non-nil result;
ensure you reference CreateLimitInput, ActiveTimeStart, ActiveTimeEnd,
CustomStartDate, CustomEndDate and Execute to locate where to hook the
expectations.
In `@internal/testhelper/time_of_day.go`:
- Around line 5-21: Move this helper into the internal/testutil package and
replace the panic-only API with a non-panicking constructor plus a thin Must
wrapper: implement a function (e.g. TimeOfDayFromString or NewTimeOfDayForTest)
that calls model.NewTimeOfDay and returns (model.TimeOfDay, error), then
reimplement MustNewTimeOfDay to call that function and panic only on error;
update package name from testhelper to testutil and update call sites to use the
new package and the non-panicking variant where appropriate.
In `@pkg/model/limit_weekly_custom_test.go`:
- Around line 540-604: The subtests in TestLimit_GetAPIResponse only inspect the
in-memory Limit fields and do not verify the serialized API contract; update
each subtest to produce and assert the actual response payload by either (a)
calling the model's response mapper (e.g., a function like Limit.ToAPIResponse /
BuildLimitResponse if one exists) and asserting the returned structure contains
the expected keys/values for LimitType (LimitTypeWeekly, LimitTypeCustom),
CustomStartDate, CustomEndDate, ResetAt, ActiveTimeStart, ActiveTimeEnd, etc.,
or (b) json.Marshal the response object and assert the resulting JSON
string/object contains the expected serialized keys and formatted values (e.g.,
"limit_type", "custom_start_date", "custom_end_date", "active_time_start",
"active_time_end", "reset_at") instead of only checking the in-memory fields on
the Limit struct.
- Around line 375-382: Add a table test that verifies the inclusive 5-year
boundary: insert a case (e.g. name "accepts period equal to 5 years") using
LimitTypeCustom where endDate is startDate.AddDate(5,0,0) (use testutil.Ptr for
both dates), set now to a date within the range, and assert expectError: false;
keep the existing failing case that uses constant.ErrLimitCustomPeriodTooLong to
ensure one-over-limit still fails; reference the
MaxCustomPeriodYears/LimitTypeCustom logic and the ErrLimitCustomPeriodTooLong
constant when locating where to add the new row.
In `@pkg/model/limit.go`:
- Around line 574-583: The Update branch never clears an existing window because
it only runs when at least one of activeTimeStart/activeTimeEnd is non-nil; add
an explicit clear flag (e.g., clearActiveWindow) to the Limit.Update signature
and handle it before the existing branch: if clearActiveWindow is true set
l.ActiveTimeStart = nil and l.ActiveTimeEnd = nil and mark updated = true;
otherwise keep the current logic (call ValidateTimeWindow when one or both
bounds provided and assign l.ActiveTimeStart/l.ActiveTimeEnd). Ensure
ValidateTimeWindow is still used when assigning new bounds and update any
callers to pass the new clear flag where a caller intends to remove the time
window.
- Around line 173-179: The CUSTOM period handling is inconsistent:
IsWithinCustomPeriod uses the full timestamp while ValidateCustomPeriod and
CalculateCustomResetAt round to day; pick a single behavior—normalize
CustomEndDate to UTC midnight everywhere. Update ValidateCustomPeriod to
truncate CustomEndDate.UTC() to 24*time.Hour (zeroing time) when validating,
update IsWithinCustomPeriod to compare against that truncated date (so the
period is inclusive/exclusive consistently), and change CalculateCustomResetAt
to return truncatedCustomEndDate.AddDate(0,0,1) (midnight UTC of the next day).
Apply the same truncation logic to any other uses (e.g., the other functions
noted) and adjust tests accordingly.
---
Outside diff comments:
In `@internal/adapters/postgres/limit_repository.go`:
- Around line 321-331: The UPDATE builder for r.tableName is missing the
reset_at column, so when limit.Update(...) recalculates dbModel.ResetAt for
custom-period edits the database isn't updated; add Set("reset_at",
dbModel.ResetAt) to the sq.Update(...) chain (where query is built using
r.tableName and dbModel) so the recalculated ResetAt is persisted alongside
name, description, max_amount, scopes, status, active_time_start/end,
custom_start/end and updated_at.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 150ce703-d7cf-4c97-ad1d-8411c45429a2
📒 Files selected for processing (11)
internal/adapters/postgres/limit_postgresql_model.gointernal/adapters/postgres/limit_repository.gointernal/adapters/postgres/limit_repository_test.gointernal/services/command/create_limit.gointernal/services/command/create_limit_test.gointernal/services/command/update_limit.gointernal/services/command/update_limit_timewindow_test.gointernal/testhelper/time_of_day.gopkg/model/limit.gopkg/model/limit_test.gopkg/model/limit_weekly_custom_test.go
- Add reset_at to limit UPDATE query (prevents DB/entity divergence) - FixedClock.Now() returns UTC matching RealClock behavior - Add boundary test for 5-year custom period limit - Rename stale TDD-RED test name to TestUpdateLimit_TimeWindow
|
This PR is very large (12 files, 2255 lines changed). Consider breaking it into smaller PRs for easier review. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/services/command/update_limit_timewindow_test.go`:
- Around line 90-92: The inline TDD comment above
TestUpdateLimit_CustomPeriod_RED is stale (it claims the test "should FAIL
initially (RED)"), so either remove or update that comment to reflect the
current behavior implemented by this PR; locate the test function
TestUpdateLimit_CustomPeriod_RED and replace the "should FAIL initially (RED),
proving the bug exists" note with a concise statement describing what the test
now verifies (e.g., "verifies custom period handling" or remove the TDD-status
phrase entirely) so future readers aren't confused.
- Around line 78-87: The test contains redundant nil guards after assert.NotNil:
remove the `if updatedLimit.ActiveTimeStart != nil { ... }` and `if
updatedLimit.ActiveTimeEnd != nil { ... }` blocks in
update_limit_timewindow_test.go and directly call assert.Equal(t, startTime,
*updatedLimit.ActiveTimeStart, ...) and assert.Equal(t, endTime,
*updatedLimit.ActiveTimeEnd, ...) (or compare values without pointer deref if
you prefer) immediately after the assert.NotNil assertions on
updatedLimit.ActiveTimeStart and updatedLimit.ActiveTimeEnd to simplify the
test.
- Around line 148-162: The test contains redundant nil checks after asserting
non-nil on updatedLimit.CustomStartDate/CustomEndDate; replace the pattern by
using require.NotNil(t, updatedLimit.CustomStartDate) and require.NotNil(t,
updatedLimit.CustomEndDate) (or keep assert.NotNil and remove the enclosing if
blocks) and then parse newStart/newEnd and directly compare expectedStart.UTC()
and expectedEnd.UTC() to updatedLimit.CustomStartDate.UTC()/CustomEndDate.UTC()
without the conditional wrappers so the test fails immediately on nil and avoids
dead code paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: be4e3572-7a6c-4f8f-aea1-0c1982890ac4
📒 Files selected for processing (5)
internal/adapters/postgres/limit_repository.gointernal/adapters/postgres/limit_repository_test.gointernal/services/command/update_limit_timewindow_test.gopkg/clock/clock.gopkg/model/limit_weekly_custom_test.go
…e tests - Replace assert.NotNil + if-guard with require.NotNil + direct assert - Rename TestUpdateLimit_CustomPeriod_RED to TestUpdateLimit_CustomPeriod - Remove stale TDD-RED comments from both test functions
|
This PR is very large (12 files, 2243 lines changed). Consider breaking it into smaller PRs for easier review. |
PR Overview
This pull request extends the
Limitmodel and its PostgreSQL repository to support new time window and custom period fields:ActiveTimeStart,ActiveTimeEnd,CustomStartDate, andCustomEndDate. These fields are now handled throughout the data access layer, including model conversion, repository CRUD operations, and related tests. The changes ensure that these new fields are properly persisted, queried, and tested.Model and Data Layer Enhancements:
ActiveTimeStart,ActiveTimeEnd,CustomStartDate,CustomEndDate) toLimitPostgreSQLModeland updated its conversion methods to map these fields between the database and the domain model. [1] [2] [3] [4]Create,Update,GetByID,List, and scan helpers) to include the new fields in SQL queries and mapping logic. [1] [2] [3] [4] [5] [6]Testing Improvements:
Service Layer Update:
CreateLimitInputstruct in the command service to accept the new fields, preparing for their use in business logic.Other Minor Changes:
Add domain constructors and validation for new limit types:
Note: CalculatePeriodKey for WEEKLY/CUSTOM will be added in PR 3 (validation pipeline) along with its tests.
Pull Request Checklist
Pull Request Type
Checklist
Please check each item after it's completed.
Additional Notes
Obs: Please, always remember to target your PR to develop branch instead of main.